Conversation
…esting Fixes #725 AI agents in automated testing mode require Playwright to verify implementations, but Docker containers had only system dependencies installed, not browser binaries. This caused verification failures with permissions errors. Changes: - Install Playwright Chromium in Dockerfile (~300MB increase) - Update docker-compose.override.yml.example with clearer Playwright documentation - Add "Playwright for Automated Testing" section to README - Document optional volume mount for persisting browsers across rebuilds Browsers are now pre-installed and work out of the box for Docker users. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPre-installs Playwright Chromium in the Docker image and documents optional browser cache; updates model mappings (Claude Opus → 4.6, adds GPT‑5.3‑Codex), adds adaptive thinking support and thinking-level UI handling, expands detailed CLI/OAuth credential detection and reporting, and updates tests and related UI/electron APIs. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Shironex, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses issue #725 by pre-installing Playwright Chromium in the Docker image, enabling automated testing without requiring additional setup. It also includes documentation updates and configuration examples to guide users on how to persist the browser cache across container rebuilds. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request pre-installs Playwright Chromium browsers in the Docker image to enable automated testing out of the box. The changes include updating the Dockerfile to install the browser, and modifying the README and docker-compose example file to document this feature and the optional persistence of the browser cache. My review focuses on improving the Docker layer caching and clarifying the documentation around using a persistent volume for the Playwright cache to avoid user confusion.
- Clean up npx cache after Playwright installation to reduce image size - Clarify README: volume mounts persist cache across container lifecycles, not image rebuilds - Add first-use warning: empty volume overrides pre-installed browsers, users must re-install with docker exec command 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docker-compose.override.yml.example`:
- Around line 24-30: Update the manual Playwright update instruction so it runs
inside the automaker-server container as the automaker user with working
directory /app (so Playwright installs to /home/automaker/.cache/ms-playwright
instead of root); specifically replace the current example command that runs as
root with one that executes the install command in the automaker-server
container using user "automaker" and working directory "/app".
In `@Dockerfile`:
- Around line 122-126: The Playwright install RUN (the command "npx playwright
install chromium") must be moved so it runs after the COPY --from=server-builder
/app/node_modules ./node_modules step; update the Dockerfile to remove the early
RUN and add a new RUN after the node_modules copy that invokes the locally
installed playwright (e.g., use ./node_modules/.bin/playwright install chromium
or npx --no-install playwright install chromium) so the pinned version from
node_modules/package-lock.json is used and no registry fetch occurs.
In `@README.md`:
- Line 347: Replace the bold subsection title "**Optional: Persist browsers
across container rebuilds**" with a proper Markdown heading (e.g., "###
Optional: Persist browsers across container rebuilds" or another appropriate
heading level) so it stops being flagged by markdownlint MD036; locate the exact
bold text in README.md and change it to the chosen heading syntax.
- Around line 362-366: Update the README's manual browser update command so it
runs as the automaker user and from /app: change the documented docker exec
invocation that currently runs npx playwright install chromium in the container
(automaker-server) to include running as user "automaker" and setting the
working directory to "/app" (i.e. add the equivalent of the docker exec flags to
set user and workdir) so that the Playwright browser cache is written to the
automaker user's cache directory rather than root.
- Move Playwright install after node_modules copy to use pinned version - Use local playwright binary instead of npx to avoid registry fetch - Add --user automaker -w /app flags to docker exec commands - Change bold text to proper heading in README (MD036 lint fix) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Shironex
added
the
Testers-Requested
Tested this PR - found an issue and submitted a fixI tested this PR on WSL2 Ubuntu and found that the Playwright install step fails during Docker build: Root cause: The Fix: I've submitted #751 which adds Feel free to:
Happy to help however is most useful! |
The Dockerfile's playwright install step requires the binary in node_modules/.bin/, but playwright was only a UI dependency. This adds @playwright/test to server devDependencies so the Docker build can successfully run `./node_modules/.bin/playwright install chromium`. Fixes the "playwright: not found" error during Docker image build. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/server/utilities.md (1)
20-20:⚠️ Potential issue | 🟡 MinorUpdate documentation to reflect current Claude model versions.
The
CLAUDE_MODEL_MAPdocumentation at line 215 is outdated. It showssonnet: 'claude-sonnet-4-20250514', but the actual code inlibs/types/src/model.tshassonnet: 'claude-sonnet-4-5-20250929'. BothCLAUDE_CANONICAL_MAPandCLAUDE_MODEL_MAPin the code consistently map to the same current version, so update the docs to match.Current code (libs/types/src/model.ts)
export const CLAUDE_MODEL_MAP: Record<string, string> = { haiku: 'claude-haiku-4-5-20251001', sonnet: 'claude-sonnet-4-5-20250929', opus: 'claude-opus-4-6', } as const; export const CLAUDE_CANONICAL_MAP: Record<ClaudeCanonicalId, string> = { 'claude-haiku': 'claude-haiku-4-5-20251001', 'claude-sonnet': 'claude-sonnet-4-5-20250929', 'claude-opus': 'claude-opus-4-6', } as const;
🤖 Fix all issues with AI agents
In `@apps/server/src/routes/setup/routes/verify-claude-auth.ts`:
- Around line 324-345: The response may leave authType undefined when authMethod
is absent; update the authenticated branch to derive authType from the effective
method (treat missing authMethod as 'api_key' like earlier logic) so authType is
always set when authenticated; specifically, in the block that sets authType
(variables: authType, authenticated, authMethod) use the effective method value
and for the 'cli' case still call getClaudeAuthIndicators() and fall back to
'cli' on error, then return that authType in the res.json response.
In `@apps/ui/src/lib/agent-context-parser.ts`:
- Around line 35-44: The formatter formatModelName currently maps 'claude-opus'
to "Opus 4.5" because the check uses model.includes('opus') before the 4.6 case;
update formatModelName so that 'claude-opus' (and any other alias that should be
4.6) is handled by the 4.6 branch—e.g., check for 'claude-opus' (or include
'claude-opus' in the same condition as 'opus-4-6') before the generic 'opus'
check so the function returns "Opus 4.6" for that alias.
🧹 Nitpick comments (6)
libs/types/src/model-display.ts (1)
146-153: Consider placing 'adaptive' before the manual levels for clarity.
adaptiveis semantically different from the manual intensity levels (low → ultrathink) — it's a mode where the model decides. Placing it last afterultrathinkcould imply it's "more" than ultrathink. Consider placing it second (afternone) to separate modes from manual levels, matching the order returned bygetThinkingLevelsForModelinsettings.tswhich returns['none', 'adaptive'].apps/server/tests/unit/lib/sdk-options.test.ts (1)
495-517: ThethinkingLevel: 'none'test is misplaced in the "adaptive thinking" describe block.The test at lines 507–516 verifies
thinkingLevel: 'none'behavior, which isn't specific to adaptive thinking or Opus 4.6. It duplicates the pattern already tested elsewhere (e.g.,createSpecGenerationOptionsat lines 368–377) and is misleadingly scoped under "adaptive thinking for Opus 4.6". Consider removing it from this block or moving it to thecreateAutoModeOptionsdescribe block above if coverage for'none'withcreateAutoModeOptionsis desired.libs/types/src/settings.ts (1)
251-269:isAdaptiveThinkingModeluses fragile string matching — consider documenting the limitation.The check
model.includes('opus-4-6')won't match future opus versions (e.g.,opus-4-7), despite the JSDoc claiming "Opus 4.6+". Also,model === 'claude-opus'only matches the exact canonical ID — not the alias'opus'or the resolved string'claude-opus-4-6'(though theincludesbranch covers the latter).This is fine for now, but consider either:
- Updating the JSDoc to say "Opus 4.6" (not "4.6+") to match the actual behavior, or
- Using a broader pattern like
model.includes('opus')if all future opus models will support adaptive thinking.📝 Suggested JSDoc fix (minimal)
/** - * Check if a model uses adaptive thinking (Opus 4.6+) + * Check if a model uses adaptive thinking (Opus 4.6) * Adaptive thinking models let the SDK decide token allocation automatically. */apps/server/src/providers/claude-provider.ts (1)
253-253: Truthy check onmaxThinkingTokenswill silently discard0.
...(maxThinkingTokens && { maxThinkingTokens })uses a truthy check. IfgetThinkingTokenBudgetever returns0(e.g., a new thinking level), it would be silently dropped. Use an explicitundefinedcheck instead.Proposed fix
- ...(maxThinkingTokens && { maxThinkingTokens }), + ...(maxThinkingTokens !== undefined && { maxThinkingTokens }),apps/ui/src/components/views/settings-view/providers/codex-model-configuration.tsx (1)
29-59: Consider derivingCODEX_MODEL_INFOfromCODEX_MODEL_CONFIG_MAPto avoid duplication.This map duplicates
id,label, anddescriptionalready defined inCODEX_MODEL_CONFIG_MAPfromlibs/types/src/codex-models.ts. Any future model addition or description change must be updated in both places. You could derive the UI info from the shared type source:import { CODEX_MODEL_CONFIG_MAP } from '@automaker/types'; const CODEX_MODEL_INFO: Record<CodexModelId, CodexModelInfo> = Object.fromEntries( Object.entries(CODEX_MODEL_CONFIG_MAP).map(([key, config]) => [ key, { id: config.id, label: config.label, description: config.description }, ]) ) as Record<CodexModelId, CodexModelInfo>;apps/server/src/routes/setup/routes/verify-claude-auth.ts (1)
324-339: Move authType derivation into a setup service.
The filesystem-based inspection is business logic; keeping the route thin will align better with the server architecture. As per coding guidelines: "Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services."
| // Determine specific auth type for success messages | ||
| let authType: 'oauth' | 'api_key' | 'cli' | undefined; | ||
| if (authenticated) { | ||
| if (authMethod === 'api_key') { | ||
| authType = 'api_key'; | ||
| } else if (authMethod === 'cli') { | ||
| // Check if CLI auth is via OAuth (Claude Code subscription) or generic CLI | ||
| try { | ||
| const indicators = await getClaudeAuthIndicators(); | ||
| authType = indicators.credentials?.hasOAuthToken ? 'oauth' : 'cli'; | ||
| } catch { | ||
| // Fall back to generic CLI if credential check fails | ||
| authType = 'cli'; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| res.json({ | ||
| success: true, | ||
| authenticated, | ||
| authType, | ||
| error: errorMessage || undefined, |
There was a problem hiding this comment.
authType stays undefined when authMethod is omitted.
The handler treats a missing authMethod as api_key earlier, but this block only sets authType when authMethod is explicitly provided. Derive it from the effective method to keep responses consistent.
🛠️ Suggested fix
- let authType: 'oauth' | 'api_key' | 'cli' | undefined;
- if (authenticated) {
- if (authMethod === 'api_key') {
+ const effectiveAuthMethod = authMethod ?? 'api_key';
+ let authType: 'oauth' | 'api_key' | 'cli' | undefined;
+ if (authenticated) {
+ if (effectiveAuthMethod === 'api_key') {
authType = 'api_key';
- } else if (authMethod === 'cli') {
+ } else if (effectiveAuthMethod === 'cli') {
// Check if CLI auth is via OAuth (Claude Code subscription) or generic CLI
try {
const indicators = await getClaudeAuthIndicators();
authType = indicators.credentials?.hasOAuthToken ? 'oauth' : 'cli';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Determine specific auth type for success messages | |
| let authType: 'oauth' | 'api_key' | 'cli' | undefined; | |
| if (authenticated) { | |
| if (authMethod === 'api_key') { | |
| authType = 'api_key'; | |
| } else if (authMethod === 'cli') { | |
| // Check if CLI auth is via OAuth (Claude Code subscription) or generic CLI | |
| try { | |
| const indicators = await getClaudeAuthIndicators(); | |
| authType = indicators.credentials?.hasOAuthToken ? 'oauth' : 'cli'; | |
| } catch { | |
| // Fall back to generic CLI if credential check fails | |
| authType = 'cli'; | |
| } | |
| } | |
| } | |
| res.json({ | |
| success: true, | |
| authenticated, | |
| authType, | |
| error: errorMessage || undefined, | |
| // Determine specific auth type for success messages | |
| const effectiveAuthMethod = authMethod ?? 'api_key'; | |
| let authType: 'oauth' | 'api_key' | 'cli' | undefined; | |
| if (authenticated) { | |
| if (effectiveAuthMethod === 'api_key') { | |
| authType = 'api_key'; | |
| } else if (effectiveAuthMethod === 'cli') { | |
| // Check if CLI auth is via OAuth (Claude Code subscription) or generic CLI | |
| try { | |
| const indicators = await getClaudeAuthIndicators(); | |
| authType = indicators.credentials?.hasOAuthToken ? 'oauth' : 'cli'; | |
| } catch { | |
| // Fall back to generic CLI if credential check fails | |
| authType = 'cli'; | |
| } | |
| } | |
| } | |
| res.json({ | |
| success: true, | |
| authenticated, | |
| authType, | |
| error: errorMessage || undefined, |
🤖 Prompt for AI Agents
In `@apps/server/src/routes/setup/routes/verify-claude-auth.ts` around lines 324 -
345, The response may leave authType undefined when authMethod is absent; update
the authenticated branch to derive authType from the effective method (treat
missing authMethod as 'api_key' like earlier logic) so authType is always set
when authenticated; specifically, in the block that sets authType (variables:
authType, authenticated, authMethod) use the effective method value and for the
'cli' case still call getClaudeAuthIndicators() and fall back to 'cli' on error,
then return that authType in the res.json response.
| export function formatModelName(model: string): string { | ||
| // Claude models | ||
| if (model.includes('opus-4-6')) return 'Opus 4.6'; | ||
| if (model.includes('opus')) return 'Opus 4.5'; | ||
| if (model.includes('sonnet')) return 'Sonnet 4.5'; | ||
| if (model.includes('haiku')) return 'Haiku 4.5'; | ||
|
|
||
| // Codex/GPT models - specific formatting | ||
| if (model === 'codex-gpt-5.3-codex') return 'GPT-5.3 Codex'; | ||
| if (model === 'codex-gpt-5.2-codex') return 'GPT-5.2 Codex'; |
There was a problem hiding this comment.
Display label for claude-opus still shows 4.5.
claude-opus is treated as Opus 4.6 elsewhere, but this formatter returns “Opus 4.5” for that alias. Consider handling the alias in the 4.6 branch to keep labels consistent.
🛠️ Suggested fix
- if (model.includes('opus-4-6')) return 'Opus 4.6';
+ if (model.includes('opus-4-6') || model === 'claude-opus') return 'Opus 4.6';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function formatModelName(model: string): string { | |
| // Claude models | |
| if (model.includes('opus-4-6')) return 'Opus 4.6'; | |
| if (model.includes('opus')) return 'Opus 4.5'; | |
| if (model.includes('sonnet')) return 'Sonnet 4.5'; | |
| if (model.includes('haiku')) return 'Haiku 4.5'; | |
| // Codex/GPT models - specific formatting | |
| if (model === 'codex-gpt-5.3-codex') return 'GPT-5.3 Codex'; | |
| if (model === 'codex-gpt-5.2-codex') return 'GPT-5.2 Codex'; | |
| export function formatModelName(model: string): string { | |
| // Claude models | |
| if (model.includes('opus-4-6') || model === 'claude-opus') return 'Opus 4.6'; | |
| if (model.includes('opus')) return 'Opus 4.5'; | |
| if (model.includes('sonnet')) return 'Sonnet 4.5'; | |
| if (model.includes('haiku')) return 'Haiku 4.5'; | |
| // Codex/GPT models - specific formatting | |
| if (model === 'codex-gpt-5.3-codex') return 'GPT-5.3 Codex'; | |
| if (model === 'codex-gpt-5.2-codex') return 'GPT-5.2 Codex'; |
🤖 Prompt for AI Agents
In `@apps/ui/src/lib/agent-context-parser.ts` around lines 35 - 44, The formatter
formatModelName currently maps 'claude-opus' to "Opus 4.5" because the check
uses model.includes('opus') before the 4.6 case; update formatModelName so that
'claude-opus' (and any other alias that should be 4.6) is handled by the 4.6
branch—e.g., check for 'claude-opus' (or include 'claude-opus' in the same
condition as 'opus-4-6') before the generic 'opus' check so the function returns
"Opus 4.6" for that alias.
Shironex
left a comment
Shironex
left a comment
There was a problem hiding this comment.
GitChorus AI Review
This is a well-structured PR that accomplishes multiple goals: (1) Docker Playwright pre-installation for automated testing, (2) Opus 4.6 model support with adaptive thinking, (3) new GPT-5.3-Codex model, (4) enhanced OAuth credential detection, and (5) improved debugging/troubleshooting UX.
Strengths:
- The adaptive thinking implementation is clean and thorough - it correctly flows through
settings.ts→sdk-options.ts→claude-provider.tswith proper handling at each layer - The credential detection enhancement with
FileCheckResult/DirectoryCheckResulttypes adds valuable debuggability - Good fix for the credential search to continue past empty/tokenless credential files (the prior behavior would stop at an empty
{}file) - Comprehensive test coverage with 761 lines of OAuth credential detection tests
- The UI auto-normalizes thinking level when switching between adaptive and non-adaptive models (add-feature-dialog.tsx)
- SDK dependency upgrades are pinned appropriately
Concerns:
- Duplicate
COPY libs/spec-parser/package*.jsonin Dockerfile (harmless but messy) - Missing trailing newline in
.gitignore - The Dockerfile RUN command for Playwright has misleading
|| echoerror handling
The PR is large but coherent. All the moving parts (types, server, UI, Docker) are updated consistently. The code quality is high with good documentation, tests, and defensive error handling. The title says "fix(docker)" but the scope is much broader than just Docker - this is really a multi-feature PR that also includes model upgrades and auth improvements.
Quality Score: 8/10 ⭐⭐⭐⭐⭐⭐⭐⭐☆☆
Findings Summary
| # | Severity | Category | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Codebase-fit | Duplicate COPY libs/spec-parser in Dockerfile | Dockerfile:28 |
| 2 | 🟡 Minor | Logic | Misleading error handling in Playwright install RUN command | Dockerfile:155 |
| 3 | 🟡 Minor | Logic | Long paths silently truncated from the right in warning box | index.ts:214 |
| 4 | 🟡 Minor | Logic | ThinkingLevelSelector defaults to non-adaptive levels when model is omitted | thinking-level-selector.tsx:22 |
| 5 | 🔵 Nit | Style | Missing trailing newline in .gitignore | .gitignore:99 |
| 6 | 🔵 Nit | Performance | Redundant filesystem checks after successful auth verification | verify-claude-auth.ts:332 |
| 7 | 🔵 Nit | Style | Verbose debug logging with inconsistent prefix style | index.ts:126 |
via GitChorus
Comments Not Placed Inline
The following findings could not be placed as inline comments (line not in diff):
Dockerfile:28— Line 28 not in diff for "Dockerfile"
.gitignore
Outdated
| data/ | ||
| .codex/ | ||
| .mcp.json | ||
| .planning No newline at end of file |
There was a problem hiding this comment.
Note
Nit - Style: Missing trailing newline in .gitignore
The .gitignore file now lacks a trailing newline. POSIX standard defines a line as ending with a newline character. Many tools warn about this, and git itself flags it in the diff. This was likely introduced when the .planning entry was added.
Problematic code:
+.planning\ No newline at end of file
Suggested fix:
Add a trailing newline after the last line:.mcp.json
.planning
| # This adds ~300MB to the image but enables automated testing mode out of the box | ||
| # Using the locally installed playwright ensures we use the pinned version from package-lock.json | ||
| USER automaker | ||
| RUN ./node_modules/.bin/playwright install chromium && \ |
There was a problem hiding this comment.
Important
Minor - Logic: Misleading error handling in Playwright install RUN command
The || echo at the end is connected to the ls command due to operator precedence, not to the entire chain. If playwright install chromium fails, the build will fail (which is correct). However, if ls fails (unlikely but possible), the echo silently masks the error. This creates a misleading success message. More importantly, the || structure makes the overall RUN step always succeed even if ls fails, which could hide a misconfigured browser install path.
Problematic code:
RUN ./node_modules/.bin/playwright install chromium && \
echo "=== Playwright Chromium installed ===" && \
ls -la /home/automaker/.cache/ms-playwright/ || echo "Playwright browsers installed"
Suggested fix:
Use explicit grouping or just remove the fallback since the ls output is informational:
```dockerfile
RUN ./node_modules/.bin/playwright install chromium && \
echo "=== Playwright Chromium installed ===" && \
ls -la /home/automaker/.cache/ms-playwright/
apps/server/src/index.ts
Outdated
| pathsCheckedInfo = ` | ||
| ║ ║ | ||
| ║ ${'Paths checked:'.padEnd(BOX_CONTENT_WIDTH)}║ | ||
| ${pathsChecked.map((p) => `║ ${p.substring(0, BOX_CONTENT_WIDTH - 4).padEnd(BOX_CONTENT_WIDTH - 4)} ║`).join('\n')}`; |
There was a problem hiding this comment.
Important
Minor - Logic: Long paths silently truncated from the right in warning box
File paths in credential checks (e.g., /home/user/.claude/.credentials.json) can be quite long, especially in Docker or CI environments. The substring(0, BOX_CONTENT_WIDTH - 4) = substring(0, 63) will silently truncate longer paths. A label like Credentials: /home/very-long-username/.claude/.credentials.json (54+ chars) with the prefix would lose the end of the path. While the box formatting requires truncation, it would be more useful to truncate from the beginning (showing the filename) rather than the end.
Problematic code:
pathsChecked.map((p) => `║ ${p.substring(0, BOX_CONTENT_WIDTH - 4).padEnd(BOX_CONTENT_WIDTH - 4)} ║`).join('\n')Suggested fix:
Consider showing the end of the path rather than the beginning when it's too long:
```typescript
const maxLen = BOX_CONTENT_WIDTH - 4;
const display = p.length > maxLen ? '...' + p.substring(p.length - maxLen + 3) : p;
pathsChecked.map((p) => {
const display = p.length > maxLen ? '...' + p.slice(-(maxLen - 3)) : p;
return `║ ${display.padEnd(maxLen)} ║`;
}).join('\n')| testIdPrefix = 'thinking-level', | ||
| model, | ||
| }: ThinkingLevelSelectorProps) { | ||
| const levels = getThinkingLevelsForModel(model || ''); |
There was a problem hiding this comment.
Important
Minor - Logic: ThinkingLevelSelector defaults to non-adaptive levels when model is omitted
When model is not provided (undefined), the fallback is an empty string. getThinkingLevelsForModel('') returns the full manual thinking levels (none, low, medium, high, ultrathink) which excludes 'adaptive'. However, the ThinkingLevelSelector component is currently not imported anywhere in the codebase, so this is a latent issue. When it eventually gets used, callers must remember to pass the model prop for adaptive thinking to work correctly with Opus 4.6.
Problematic code:
const levels = getThinkingLevelsForModel(model || '');Suggested fix:
This is acceptable as-is since the component isn't yet used, but consider documenting the required model prop more prominently:
```tsx
/** Model ID is required for correct thinking level filtering.
* Without it, adaptive thinking won't be available for Opus 4.6. */
model?: string;| } else if (authMethod === 'cli') { | ||
| // Check if CLI auth is via OAuth (Claude Code subscription) or generic CLI | ||
| try { | ||
| const indicators = await getClaudeAuthIndicators(); |
There was a problem hiding this comment.
Note
Nit - Performance: Redundant filesystem checks after successful auth verification
After successfully verifying authentication via a full SDK query (which already proved auth works), this code calls getClaudeAuthIndicators() again just to determine the auth type. This performs additional filesystem reads (checking settings files, stats cache, projects dir, credential files) on every successful CLI auth verification. While not a significant performance issue, the indicator data was already fetched during the startup check. Consider caching or reusing that data.
Problematic code:
const indicators = await getClaudeAuthIndicators();Suggested fix:
This is acceptable for now since it only runs during setup verification (not a hot path). Could be improved by caching the indicators or passing them through context, but low priority.| const hasAnthropicKey = !!process.env.ANTHROPIC_API_KEY; | ||
| const hasEnvOAuthToken = !!process.env.CLAUDE_CODE_OAUTH_TOKEN; | ||
|
|
||
| logger.debug('[CREDENTIAL_CHECK] Starting credential detection...'); |
There was a problem hiding this comment.
Note
Nit - Style: Verbose debug logging with inconsistent prefix style
The new debug logging in the credential check is verbose - there are 6 debug log calls added throughout the auth detection flow. While helpful for debugging, the [CREDENTIAL_CHECK] prefix is inconsistent with other logging patterns in the file (which don't use bracketed prefixes). This is a minor style concern.
Problematic code:
logger.debug('[CREDENTIAL_CHECK] Starting credential detection...');
logger.debug('[CREDENTIAL_CHECK] Environment variables:', {
hasAnthropicKey,
hasEnvOAuthToken,
});Suggested fix:
Consider using a consistent prefix format, e.g., using the logger's category instead:
```typescript
const credLogger = createLogger('CredentialCheck');
credLogger.debug('Starting credential detection...');Or keep it as-is since it's debug-level only and useful for troubleshooting.
- .gitignore: add missing trailing newline - Dockerfile: remove misleading || echo fallback in Playwright install - index.ts: truncate long paths from beginning instead of end in warning box - verify-claude-auth.ts: use effectiveAuthMethod to prevent undefined authType - agent-context-parser.ts: handle claude-opus alias as Opus 4.6 - thinking-level-selector.tsx: improve model prop documentation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Fixes #725
AI agents in automated testing mode require Playwright to verify implementations, but Docker containers had only system dependencies installed, not browser binaries. This caused verification failures with permissions errors.
Changes:
Browsers are now pre-installed and work out of the box for Docker users.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores